Skip to content
This repository was archived by the owner on Jun 20, 2024. It is now read-only.

Fix docker api proxy smoke-tests after docker-py change (on 1.0 branch) #1367

Merged
merged 5 commits into from
Aug 28, 2015

Conversation

bboreham
Copy link
Contributor

Docker-py was changed upstream to run using a test harness Pytest.

Also I have seen some failures due to the 'busybox' image not being found on the test host.

@bboreham
Copy link
Contributor Author

I have no idea why particular test is failing on 1.0 and not on master:

>       self.assertEqual(container_log_config['Config'], {})
E       AssertionError: None != {}

@rade
Copy link
Member

rade commented Aug 27, 2015

self.assertEqual(container_log_config['Config'], {})

So that's in test_valid_no_config_specified, which was introduced recently.

@rade
Copy link
Member

rade commented Aug 27, 2015

@paulbellamy can you look into that "breaks on 1.0, fine on master" issue? I've spent a few minutes checking whether anything significant has changed between 1.0 and master in how we manipulate the HostConfig (which is where the log config resides) on create, but it all looks pretty similar.

@paulbellamy paulbellamy self-assigned this Aug 27, 2015
@paulbellamy
Copy link
Contributor

I believe it is because in the createContainerInterceptor we decode, modify, and re-encode the request. Because go has limited nil/zero semantics the go dockerclient encodes the request slightly differently. Sending a null instead of {}.

To completely avoid this we would have to refrain from parsing the request using the go docker client, and just parse it as a set of maps (which would all-around be safer anyway). However that is a bigger change than should go in 1.0.

@rade
Copy link
Member

rade commented Aug 27, 2015

I believe it is because in the createContainerInterceptor we decode, modify, and re-encode the request.

That's exactly the kind of thing I was looking for in the proxy code, but afaict we do that in both 1.0 and master, so it doesn't explain the difference in behaviour.

@paulbellamy
Copy link
Contributor

There is no change. It also fails on master, but master's test matching regex is wrong.

@rade
Copy link
Member

rade commented Aug 28, 2015

master's test matching regex is wrong

File an issue.

To completely avoid this we would have to refrain from parsing the request using the go docker client, and just parse it as a set of maps (which would all-around be safer anyway).

Is that what docker itself does?

I do rather wonder whether the test is too strict, or even just wrong. Note that it actually sends None and expects to get back {}. Which is weird.

@paulbellamy
Copy link
Contributor

Even when you say None python still sends {}.

@paulbellamy
Copy link
Contributor

Edit: disregard this, was not what was happening.

@rade
Copy link
Member

rade commented Aug 28, 2015

So how do the docker client and docker itself differ in decoding the request?

@paulbellamy
Copy link
Contributor

Hold on actually. Needs more research..

This is a temporary fix for a failure in docker-py tests:
"test_valid_no_config_specified".
@paulbellamy paulbellamy removed their assignment Aug 28, 2015
@paulbellamy
Copy link
Contributor

Ok, have added a temporary fix for these tests, where we don't replace the request body unless we modify it.

Opened #1371 to deal with the underlying issue, which is a bigger change than I would like for 1.0.

@@ -5,6 +5,7 @@
start_suite "Run docker-py test suite against the proxy"

docker_on $HOST1 pull joffrey/docker-py >/dev/null
docker_on $HOST1 pull busybox >/dev/null

This comment was marked as abuse.

@rade rade self-assigned this Aug 28, 2015
@rade rade removed their assignment Aug 28, 2015
@rade
Copy link
Member

rade commented Aug 28, 2015

@paulbellamy @bboreham I have made a couple of small tweaks. Please check they are ok. Then squash and merge and do the 1.0->master merge. All LGTM :)

rade added a commit that referenced this pull request Aug 28, 2015
Fix docker api proxy smoke-tests after docker-py change
@rade rade merged commit 8184d29 into 1.0 Aug 28, 2015
@rade rade modified the milestone: 1.0.3 Aug 29, 2015
@rade rade deleted the 1.0-python-call-changed branch August 31, 2015 09:49
bboreham added a commit that referenced this pull request Sep 1, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants